feat: Digest mail notifications - eXIP7.3.0.22 - integration into feature/mips - #6093
Conversation
ff1596d to
31a8c05
Compare
31a8c05 to
f380965
Compare
azayati
left a comment
azayati
left a comment
There was a problem hiding this comment.
AI review — Round #1 — eXIP 7.3.0.22 Digest mail notifications (social)
Reviewed as one delivery with the 18 sibling PRs (exip-7.3.0.22-mips), against Tech Spec note 50469 and board 8372, at head f380965b. Every finding below was verified in the source at that commit. The engine findings (claim atomicity, makeDigest removal, Liquibase rollback) are reported on commons#786.
🟢 Nit — a few small ones
DigestRest.java:42,65: the controller autowiresio.meeds.commons.digest.service.DigestLabelResolver, an implementation-package component of another module, to translate category labels. Expose the labelled categories through theDigestServiceAPI (e.g.getCategories(Locale)) and keep REST on the API package.DigestRestTest.java:98-133: five tests, GET and the admin PATCH only. The userPATCH settingshas no REST-level pin: owner happy path, 400 on a frequency with no category, 403 when the admin switch is OFF (backend-spring §5 exception → status contract).NotificationChannels.vue:47-49:created()callsgetDigestSettings().then(...)without.catch— a failing GET is an unhandled rejection while the switch silently stays OFF (UserSettingNotifications.vue:146does catch it).NotificationAdministration_en.properties:9: "Allow to use digest mail notification" vs the board US01 / spec wording "Allow users to set digest mail notification". US01 is Tested & Validated on the FB, so just confirm this is the intended label.Profile.java:65:@Deprecated(since = "7.3.0")onTIME_ZONE— backend-spring §6 also asks the comment to say whether it is for removal.UserSettingDigestDrawer.vueis 219 lines (norm ≤ 200 per component, header included — borderline).UserSettingNotifications.vue:144andNotificationChannels.vue:47both fetch the fullGET /digest/settings(categories + labels + choices) only to readdigestAllowed; cheap, a lighter read would do.
Verified conform: SocialApplication adds io.meeds.commons.digest to both scanBasePackages and @EnableJpaRepositories (the hosted-module ruling); DigestRest is Spring MVC under /social/rest/notifications/digest, @Secured("users") on GET/PATCH settings and @Secured("administrators") on the admin PATCH, owner = request.getRemoteUser() only, IllegalArgumentException → 400 with the message code, no endpoint added to the legacy JAX-RS service, @SpringBootTest + @MockitoBean + MockMvc test harness; the enrollment timezone snapshot comes from the profile (UserTimeZoneService.getUserTimeZone), never from the client. Timezone sync: Profile.USER_TIME_ZONE = "user.timeZone" added and TIME_ZONE deprecated not removed; UserTimeZoneService.saveUserTimeZone validates ZoneId.of, saves the org profile attribute and broadcasts social.timeZone.saved with (username, zoneId) — the exact contract commons' DigestTimeZoneListener reads; TimeZoneRest POST text/plain, 400 on an unknown zone; UserTimeZoneSync.js posts only when the browser zone differs from the printed one; the head template validates the stored value with ZoneId.of before printing it and the profile read goes through the cached UserProfileHandler; the module is required from UISocialBottomContainer.gtmpl for logged users only. SocialDigestLinePlugin: the 4 spec plugin ids, parameters are the very keys the plugins write (SPACE_ID/PROFILE/REQUEST_FROM/POSTER/ACTIVITY_ID), vanished space/activity → no line, invitation → /portal/s/{id}, join request → /portal/s/{id}/members (EXO-90021), activities via the redirect URL, titles stripped of HTML and abbreviated, services looked up lazily; 9 tests. Categories spaces (10) and feed (20) with the right plugin ids, label and line keys present in Notification_en.properties, text only, 47 locales touched consistently. SpaceNotificationImpl.discardDigestItems is glue only, called for invitation cancellation and join-request withdrawal, "spaceId" == the key both plugins store, failures never block the on-site removal. Vue: the only new drawer is UserSettingDigestDrawer.vue (+ entry + categories), fetch only in js/DigestService.js and NotificationAdministration.js, $digestService on Vue.prototype, entry and drawer under v-if="digestAllowed" from the parent, exo-drawer reused, pre-select all on first enable, unchecking all switches the frequency off, Apply disabled on an enabled frequency with no category, snackbar text == board US05. Legacy cleanup complete: EmailDigestChoice, the digest fields of UserNotificationSettings, the digest branch and DigestDailyPlugin fallback of NotificationSettingsRestService, getMessageInSpace/getMessageByIds, every makeDigest override including the Post Reporting postReport builder that landed on mips, both test gtmpl, the digestMailNotification feature reads in 4 Vue files, the Notification.digest.* and UINotification.label.{Daily,Weekly,Never,Monthly,selectBox-mail} keys — git grep on *.java *.vue *.js *.gtmpl is empty.
What this PR does well: the timezone sync moves into the platform with the old constant deprecated rather than deleted, the event contract documented at both ends, and every agenda caller removed and verified absent; the drawer follows the "fetch in services, v-if from the parent" rule throughout; the cleanup found and removed the postReport override that a silent git merge had let through.
Classification: N1 (part of the eXIP: new REST endpoints, the hosted commons engine, mass email). This PR must be approved by an Architect / Senior Developer who knows it is N1 and is not its author — never on this AI review alone. Merge after commons#786 (Nexus snapshot published). The Knowledge: line of the body is still TODO.
🤖 Generated with Claude Code
|
Round #1 — author's answers to the nits (commit
🤖 Generated with Claude Code |
AI review — Round #2 (follow-up)Fresh reviewer over the delta since round 1 ( Round-1 findings
New findings of round 2None. Verified conform this round: Classification: N1 (part of the eXIP: new REST endpoints, the hosted commons engine, mass email). Approver an Architect / Senior Developer other than the author, never on this AI review alone. Merge after commons#786 (Nexus snapshot published). 🤖 Generated with Claude Code |
AI review — Round #3 (final) — close-outThe closing reviewer (fresh, round 3) re-verified every finding of this PR in the source at
Verified conform: hosted-module bootstrap ( What remains is not code: the Classification: N1 (part of the eXIP: new REST endpoints, the hosted commons engine, mass email). This PR must be approved by an Architect / Senior Developer who knows it is N1 and is not its author, never on this AI review alone. Merge after commons#786 (Nexus snapshot published). 🤖 Generated with Claude Code |
3312d01 to
ca80c72
Compare
1d31824 to
b83081a
Compare
AI review — Round #4 (independent review, Architects Lead's reviewer)Independent pass at Author's rounds 1–3 — status re-verified in the source at head
New findings — nothing 🟡 or above on this PR🟢 Nit — a few small ones
Verified conform — spec rulings and sensitive points checked in the code
What the PR does wellThe timezone synchronisation becomes a platform service with a validated input, an event contract documented at both ends and every agenda caller removed and verified absent; the drawer keeps the "fetch in services, Classification: N1 (Liquibase schema and drops, JPA entities, the notification dispatcher, new REST endpoints, mass email). This PR must be approved by an Architect / Senior Developer who knows it is N1 and is not its author — never on this AI review alone. The 🤖 Generated with Claude Code |
boubaker
left a comment
boubaker
left a comment
There was a problem hiding this comment.
AI review — Round #4 (independent review) — addendum
One more finding on the timezone synchronisation wiring, anchored inline on the module declaration. The rest of Round #4 stands as posted above.
Classification: N1 (unchanged). This PR must be approved by an Architect / Senior Developer who knows it is N1 and is not its author — never on this AI review alone.
🤖 Generated with Claude Code
…P7.3.0.22 (#6076) eXIP 7.3.0.22 Digest Mail Notifications — feedback task EXO-90021 from the functional tests. The space lines of the digest linked to the legacy space redirections (`/rest/social/notifications/redirectUrl/space/<id>` and `space_members`), which build `/portal/g/:spaces:…` URLs and land on a not found page. They now link to the space pages of the platform: - "{actor} invited you to join {space}" → `<domain>/portal/s/<spaceId>`, the space page, which lets the user accept the invitation while it is pending and opens the space once accepted; - "{actor} requested to join {space}" → `<domain>/portal/s/<spaceId>/members`, where the manager handles the request. Tests: `SocialDigestLinePluginTest` (9 green). Classification: **N2**. Knowledge: none. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> (cherry picked from commit dc5ba3a) (cherry picked from commit cc9d4e3)
…ed categories API, REST tests - EXO-89484_EXO-89482 - eXIP7.3.0.22 - The "digest not allowed by the administrator" rule is enforced by DigestService.saveUserSettings; DigestRest only maps IllegalAccessException to 403 next to IllegalArgumentException to 400. - DigestRest reads the labelled categories from DigestService.getCategories(Locale) instead of the commons DigestLabelResolver implementation class. - Four DigestRestTest cases for the user PATCH (anonymous, owner with his profile timezone, 403, 400). - The admin switch label follows the board wording; the digestAllowed loader of the administration app catches a failing GET; Profile.TIME_ZONE deprecation says it is not for removal. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> (cherry picked from commit 1d31824)
b83081a to
7725d70
Compare
…XO-90072 - eXIP7.3.0.22 Prior this change, the social notification plugins declared "daily" or "weekly" values in their defaultConfig collections: twelve in the webapp configuration, ten in the notification test resource and three in the service test resource. Both readers of that collection — NotificationPluginContainer.getDefaultActivePlugins and JPAUserSettingServiceImpl.getDefaultSettings — compare each value to UserSetting.FREQUENCY.INSTANTLY, and FREQUENCY holds INSTANTLY alone since the legacy digest engine was removed, so getFrequecy("daily") returns null and the value is silently ignored: configuration left dead by that removal. After this commit, those 25 values are gone and only "Instantly" remains where it was declared. The defaultConfig field of NewUserPlugin, and of LikePlugin in the notification test resource, held a dead value alone and is dropped whole rather than left as an empty collection. The default activation of every plugin is unchanged: an ignored value, an empty list and an absent field give the same result in both readers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…XO-89484 - eXIP7.3.0.22 Prior this change, the timezone synchronization shipped as a baseGRP module named userTimeZoneSync and was wired by an explicit require in UISocialBottomContainer.gtmpl. The portal filters the defined modules on the "BaseExtension" suffix (Utils.includeExtensions, called once per page by UIPortalApplicationChildren.gtmpl), so a module without that suffix is never picked up by the hook and needs the page-template coupling the hook exists to avoid; agenda's agendaBaseExtension, which this module replaces, was loaded that way and required nowhere. After this commit, the module is named userTimeZoneBaseExtension, stays in baseGRP and is loaded by the hook: require.js registers every defined module name in window.requireJsModules, the whole baseGRP script is served on every page, and includeExtensions('BaseExtension') requires the module and calls its init() when it exposes one. The require block of UISocialBottomContainer.gtmpl is removed, leaving that template as it is on the merge target, and the head template comment now names the module and how it is loaded. The script itself is unchanged and still guards on eXo.env.portal.userName. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Reply to Round #4 (#6093 (comment)). Fixed at head 🟢 Dead
|
AI review — Round #5 (final)Independent review, Architects Lead's reviewer, at head Status of Round #4 and its addendum
Status of the author's rounds 1–3Unchanged from Round #4: ✅ re-verified then, the drawer length / double GET decline accepted. New findingsNone. The two fix commits were reviewed as new code: the rename keeps the module in the group the hook scans, and the removal touches no All findings from every round are resolved or accepted on this PR; nothing outstanding from the AI review side. The Classification: N1 (part of the eXIP: new REST endpoints, the hosted commons engine, mass email). The approval below is the human reviewer's own, recorded after his manual review; author ≠ approver holds. 🤖 Generated with Claude Code |
2 participants
eXIP 7.3.0.22 — Digest mail notifications · integration into
feature/mipsThe administration switch, the user-settings entry and drawer, the Spring REST controller of the digest, the timezone synchronization moved from agenda into the platform (new endpoint + service), the spaces and feed categories with their email lines, and the cleanup of the legacy digest leftovers.
The 7 commit(s) replayed from
feature/experience5212566266feat: Implement admin option to allow digest mail notifications - EXO-89482 (feat: Implement admin option to allow digest mail notifications - EXO-89482 - eXIP7.3.0.22 #6017)3b28158622feat: List digest mail notification option to users - EXO-89483 (feat: List digest mail notification option to users - EXO-89483 - eXIP7.3.0.22 #6024)bc5a61b9effeat: Digest mail notifications settings drawer - EXO-89484 (feat: Digest mail notifications settings drawer - EXO-89484 - eXIP7.3.0.22 #6027)d1ff2bfb8ffeat: Digest capture on the social side, recap snackbar and discard on cancellation - EXO-89485 (feat: Digest capture on the social side, recap snackbar and discard on cancellation - EXO-89485 - eXIP7.3.0.22 #6056)7c9a929aaffeat: Digest email lines for spaces and feed - EXO-89486_EXO-89487 (feat: Digest email lines for spaces and feed - EXO-89486_EXO-89487 - eXIP7.3.0.22 #6072)cc9d4e373bfix: The digest space lines link to the space pages - EXO-90021 (fix: The digest space lines link to the space pages - EXO-90021 - eXIP7.3.0.22 #6076)f380965baefix: Remove the legacy digest leftovers of social - EXO-90072 (fix: Remove the legacy digest leftovers of social - EXO-90072 - eXIP7.3.0.22 #6083)148 files changed, 2238 insertions(+), 4932 deletions(-)
How this branch was built
exip-7.3.0.22-mipsstarts fromorigin/feature/mipsand replays only the commits taggedeXIP7.3.0.22(git cherry-pick -x, the original reference is in every message). The FB version-bump commit (Task-87990) and the commits of the other eXIPs present onfeature/experienceare deliberately left out — nopom.xmlis touched.Merge order
makeDigestis still an abstract method ofcommons-apionfeature/mips; the other 18 PRs remove their overrides, so they only compile once commons is merged: commons#786 first (wait for the Nexus snapshot), then social#6093, then the 17 addons in any order.Classification
N1 for the whole eXIP (Liquibase schema and JPA entities, the commons notification dispatcher, new REST endpoints, mass email sending). Approver ≠ author: this PR must be approved by an Architect / Senior Developer who knows it is N1, not on the AI review alone.
Already validated on
feature/experienceEvery US of the board (project 8372) is "Tested & Validated" by the PO, the legacy engine cleanup (EXO-90072, 19 repositories) included. Full functional test plan: capture, daily and weekly contents, timezones, catch-up at startup, safety cleanup, and non-regression of the instant notifications.
Knowledge: TODO — eng-standards PR to open (
/domain-doccommons + social) before leaving draft🤖 Generated with Claude Code